Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CDF-23593] 😃 Refactor dump method #1313

Open
wants to merge 43 commits into
base: refactor-tool-config
Choose a base branch
from

Conversation

doctrino
Copy link
Collaborator

@doctrino doctrino commented Dec 20, 2024

Description

Changes the ResourceLoaders

Removed the following method

    def are_equal(
        self,
        local: T_WriteClass,
        cdf_resource: T_WritableCogniteResource,
        return_dumped: bool = False,
        ToolGlobals: CDFToolConfig | None = None,
    ) -> bool | tuple[bool, dict[str, Any], dict[str, Any]]:
    ...

Changed the signature of the following three methods

    def load_resource_file(
        self, filepath: Path, ToolGlobals: CDFToolConfig, is_dry_run: bool = False
    ) -> T_WriteClass | T_CogniteResourceList: ...

    def load_resource(
        self, resource: dict[str, Any] | list[dict[str, Any]], is_dry_run: bool = False, filepath: Path | None = None
    ) -> T_WriteClass | T_CogniteResourceList: ...

    def dump_resource(self, resource: T_WritableCogniteResource, local: T_WriteClass) -> dict[str, Any]: ...

To this:

    def load_resource_file(
        self, filepath: Path, environment_variables: dict[str, str | None] | None = None
    ) -> list[dict[str, Any]]: ...

    def load_resource(self, resource: dict[str, Any], is_dry_run: bool = False) -> T_WriteClass: ...

    def dump_resource(self, resource: T_WritableCogniteResource, local: dict[str, Any]) -> dict[str, Any]: ...

Notice that this signature has fewer arguments than the original. It simplifies the implementations, in addition, by returning the list[dict[str, Any]] in the load_resource_file the methods are more flexible which means that they can be used in the cdf modules pull command.

Finally, realizes that loading from file, and figuring out what to create, update, and remain unchange, had to be done in one step, as now we dump the CDF resources into the same format as the local file.

Introduced a ResourceWorker to do this. note I am not happy with the name, I think it should ideally be changed to ResourceLoader and rename the ResourceLoaders to something like StandardizedResource as this does a lot more than just loading individual files, dumping, CRUD, iteration, authorization as well.

class ResourceWorker
    def __init__(self,  loader: ResourceLoader):
        self.loader = loader

    def load_files(
        self, sort: bool = True, directory: Path | None = None, read_modules: list[ReadModule] | None = None
    ) -> list[Path]: ...

    def load_resources(
        self,
        filepaths: list[Path],
        return_existing: bool = False,
        environment_variables: dict[str, str | None] | None = None,
        is_dry_run: bool = False,
        verbose: bool = False,
    ) -> (
        tuple[T_CogniteResourceList, T_CogniteResourceList, T_CogniteResourceList, list[T_ID]]
        | tuple[T_WritableCogniteResourceList, list[T_ID]]
    ): ...

Checklist

@doctrino doctrino force-pushed the refactor-dump-method branch from 8e4a171 to 5695239 Compare December 20, 2024 21:39
@doctrino doctrino force-pushed the refactor-dump-method branch from e946591 to c5d90f4 Compare December 21, 2024 14:33
@doctrino doctrino force-pushed the refactor-dump-method branch from bccb947 to 6274a74 Compare December 21, 2024 14:44
@@ -25,6 +25,7 @@ Changes are grouped as follows:
### Fixed

- No more warning about missing `.env` file when running in `Google Cloud Build`.
- When deploying a `Sequence` resource, Cognite Toolkit now replaces `dataSetExternalId` with `dataSetId`.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found this bug in the refactoring.

@@ -82,7 +83,7 @@ def external_id(
id: int | Sequence[int],
) -> str | list[str]:
ids = [id] if isinstance(id, int) else id
missing = [id_ for id_ in ids if id not in self._reverse_cache]
missing = [id_ for id_ in ids if id_ not in self._reverse_cache]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another bug introduced in last refactoring.

Copy link
Collaborator

@ronpal ronpal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introduced a ResourceWorker to do this. note I am not happy with the name, I think it should ideally be changed to ResourceLoader and rename the ResourceLoaders to something like StandardizedResource as this does a lot more than just loading individual files, dumping, CRUD, iteration, authorization as well.

Agree that we should rename, let's revisit later. Apart from that, great progress 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants